Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 1 | Bug Report: Extra long blooms?#147
Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 1 | Bug Report: Extra long blooms?#147HassanOHOsman wants to merge 10 commits intoCodeYourFuture:mainfrom
Conversation
HassanOHOsman
added
⚙️ bug
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Convince me of your approach (good luck!) or make it fail not trim!
front-end/components/bloom-form.mjs
Outdated
| const originalText = submitButton.textContent; | ||
| const textarea = form.querySelector("textarea"); | ||
| const content = textarea.value.trim(); | ||
| const content = textarea.value.trim().slice(0, 280); |
There was a problem hiding this comment.
I'm not convinced. Should too long a bloom fail, or should you truncate it and accept it? I think it should fail.
There was a problem hiding this comment.
To be honest, at the time I was in two minds whether or not to handle the too long bloom with an error which lead to it not being submitted. However, somehow eventually I resorted to chopping the extra characters. I perhaps thought this approach would me more ideal since it's already published bloom.
I understand now I have to fail it altogether. I will working on it. Thank you.
github-project-automation
bot
moved this from Backlog
to In Progress
in Purple Forest Kanban
OracPrime
added
Reviewed
… will later be replaced with a logic that fails the bloom altogeather.
… the console. Use it to skip it while fetching all blooms from backend
…en blooms fetched from backend
HassanOHOsman
added
Needs Review
|
AS bloom - the one over 280 character is now hidden. Not sure if it's the ideal approach but I did use the bloom id from the error message showed up on my browser and used it to skip over the culprit bloom when blooms are being fetched. |
|
I understand very well that, ideally, there should've been a condition that'd check if a bloom was over 280 characters and if so it'd be skipped over. Having said that this just didn't work for some reason when i did it. |
|
UPDATE: Well, it has worked now - I replaced the bloom ID-based logic to a conditional logic that checks if bloom is over 280 char and skips over it if that's the case. I had to disconnect from all ports and reconnect again. |
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
You seem to be prevent overlong blooms being displayed when reading them back from the database. This is the wrong time to be doing it. You need to prevent overlong blooms being added.
I'm also concerned that your code refers to a non-existent function. Have you tested this?
backend/data/blooms.py
Outdated
|
|
||
| def get_bloom(bloom_id: int) -> Optional[Bloom]: | ||
| with db_cursor() as cur: | ||
| with db_get_bloomcursor() as cur: |
There was a problem hiding this comment.
This function doesn't exist does it?
There was a problem hiding this comment.
I have not implemented unit tests. However, i have manually tested what i did through the browser. Everything seemed fine, the culprit bloom wasn't/isn't displayed (which - I now know - is not the right way to do it), clicked on different buttons, sent a bloom so basically did different operations and no error messages showed up whatsoever in the console. I've attached screenshots for reference.
As for the non-existent function, I can't recall how that happen but I got it to how it was.
OracPrime
added
Reviewed
2 participants
Self checklist
This PR fixes AS's extra long bloom bug by ensuring it respects the 280-character limit. Changes include: